-
Notifications
You must be signed in to change notification settings - Fork 933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Script reference consistency #2841
Conversation
…tartscript; move scriptrunning and stopscript
Hm. Not sure. That's a pretty big refactoring. Does this change actually fixes any issues or is it just code maintenance? If it is the later then I am not in favour of it. Please remember that the old scripting engine will be removed eventually. There is little point in polishing it up. |
Meanwhile I like it to be honest, it may take a while before it's rewritten in some way and IMO it would decomplicate the rewriting a lot if all function opcodes are handled in a generic way. |
I would like to take this in to consideration. The effort has been done, it is simple/elegant/generic. Agreed that we do plan on deprecating and replacing the scripting engine, but I think the existing one will still be around for awhile. |
Aside from removing code bloat, I think Capo's argument might be the best reason to consider merging this. Having everything nice and consistent should aid future refactoring/switching to Lua.
No issues, just maintenance. Mostly because it took me a while to find StartScript when I started #2648.
Arguably only the half the effort's been done. (The larger half, admittedly.) Figured I'd get some feedback before going crazy. |
Okay. I agree that we don't have reason not to merge this one. But I still think it is not a good use of time to continue on this refactoring task (which of course doesn't mean you can't do it, if you really want to). Also, from what I remember MessageBox is kinda tricky. Not sure if it even can fit into the current system without further enhancements. |
I always find deleting code to be pretty cathartic, so I'll probably have a look at the other functions I mentioned soonish. And after that... who knows; maybe mutable base records so the ordinators can do their thing. |
Is this then still WIP? |
Yes, I'm gonna have a look at the other functions I mentioned and (hopefully) add all of them to this. I also still need to verify I didn't break anything with StartScript, StopScript, and ScriptRunning. It's probably fine, but that's not really good enough. (The others I did test already.) |
Make sure to keep in scope, we are not going to be adding to or extending mwscript. We could use this as a basis for obscript however. Something to keep in mind. :) |
Yeah, I had a quick look and I'll just leave it be. Not worth it for now. So with MenuMode, Random, and GetSecondsPassed done and MessageBox skipped... I'm gonna say this bit of refactoring is done. Unless someone has very strong feelings about GetSquareRoot. |
This assert fails now:
|
I have a suspicion that it is because of |
There's an inconsistency in how implicit and explicit references are handled for a few functions. These functions are defined in components rather than apps/openmw meaning they can't access the reference helpers defined in apps/openmw/mwscript/ref.hpp. So my aim here is to downgrade these functions from language constructs to regular functions and move them into apps/openmw - where the vast majority of their friends reside.
The ones that're interesting for references are:
For the sake of keeping the scripting functions grouped I also moved ScriptRunning and StopScript.
In all honesty, the remaining functions in installopcodes.cpp should probably also be moved. That's to say, these ones:
So... how does everyone feel about this?